Skip to content

Move query code outside macros and store query jobs separately from query results #50102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 27, 2018
Merged

Move query code outside macros and store query jobs separately from query results #50102

merged 4 commits into from
Apr 27, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Apr 20, 2018

@pietroalbini pietroalbini added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2018
@michaelwoerister
Copy link
Member

Thanks, @Zoxc!

Do you have any measurements how splitting the map affects memory usage and performance? In the cache miss case, it leads to a few more hashtable lookups than the current version, I think.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 20, 2018

It uses 450MB when compiling syntex_syntax vs. 456MB before. Compile times seems to be slightly reduced, although that may just be due to noise.

@TimNN
Copy link
Contributor

TimNN commented Apr 24, 2018

Ping from triage @michaelwoerister ! This PR needs your review.

@michaelwoerister michaelwoerister mentioned this pull request Apr 25, 2018
Closed
@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 25, 2018

@bors try

@bors
Copy link
Collaborator

bors commented Apr 25, 2018

⌛ Trying commit 87956b5a5eaee00b89297551c1dd75e1ff632650 with merge 34c50be0a1c561e15f7d02e06c7e47b48a40e444...

@bors
Copy link
Collaborator

bors commented Apr 25, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 25, 2018

@Mark-Simulacrum I'd like a perf run here

@Mark-Simulacrum
Copy link
Member

Perf queued.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 26, 2018

I did some microbenchmarks here too:
Before:

  time: 0.827; rss: 36MB        hot query (usize)
  time: 0.942; rss: 170MB       cold query (usize)
  time: 0.911; rss: 170MB       hot query (dummy DefId)
  time: 0.972; rss: 304MB       cold query (dummy DefId)
  time: 0.960; rss: 304MB       hot query (DefId)
  time: 0.953; rss: 438MB       cold query (DefId)

After:

  time: 0.367; rss: 36MB        hot query (usize)
  time: 0.717; rss: 136MB       cold query (usize)
  time: 0.498; rss: 136MB       hot query (DefId)
  time: 0.795; rss: 237MB       cold query (DefId)
  time: 0.379; rss: 237MB       hot query (dummy DefId)
  time: 0.706; rss: 338MB       cold query (dummy DefId)

Before (with incremental compilation):

  time: 0.958; rss: 40MB        hot query (usize)
  time: 3.165; rss: 638MB       cold query (usize)
  time: 1.080; rss: 638MB       hot query (dummy DefId)
  time: 3.286; rss: 1237MB      cold query (dummy DefId)
  time: 1.088; rss: 1237MB      hot query (DefId)

After (with incremental compilation):

  time: 0.511; rss: 40MB        hot query (usize)
  time: 2.584; rss: 605MB       cold query (usize)
  time: 0.680; rss: 605MB       hot query (DefId)
  time: 0.478; rss: 605MB       hot query (dummy DefId)
  time: 2.796; rss: 1170MB      cold query (dummy DefId)

@michaelwoerister
Copy link
Member

Looks very promising, @Zoxc!

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 27, 2018

Looks green.
http://perf.rust-lang.org/compare.html?start=25749ad66de71ffb46e6b04fb0cccf1ae4fc5ed5&end=34c50be0a1c561e15f7d02e06c7e47b48a40e444&stat=instructions%3Au

I think I'm going to close #50067 in favor of this.

@michaelwoerister
Copy link
Member

Thanks, @Zoxc! The first two commits look great. The third one I'm a little ambiguous about because of the additional table lookups. But since performance seems to improve overall, I don't have any real objections. All in all this PR cleans up the core query code considerably! No we just have to merge dep_node.rs into plumbing.rs and all will be great in query land ;)

r=me with the comments from #50067 addressed.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 27, 2018

@bors r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Apr 27, 2018

📌 Commit f678bf0 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2018
@michaelwoerister
Copy link
Member

🎉

@bors
Copy link
Collaborator

bors commented Apr 27, 2018

⌛ Testing commit f678bf0 with merge 3c43aa5...

bors added a commit that referenced this pull request Apr 27, 2018
@bors
Auto merge of #50102 - Zoxc:query-nomacro, r=michaelwoerister
3c43aa5 
Move query code outside macros and store query jobs separately from query results

Based on #50067

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented Apr 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 3c43aa5 to master...

@bors bors merged commit f678bf0 into rust-lang:master Apr 27, 2018
@bors bors mentioned this pull request Apr 27, 2018
Closed
@Zoxc Zoxc deleted the query-nomacro branch April 27, 2018 21:47
@Zoxc Zoxc mentioned this pull request Apr 30, 2018
Closed
@nikomatsakis nikomatsakis mentioned this pull request May 4, 2018
Open
31 tasks
This was referenced Apr 16, 2019
Closed
Closed
@lqd lqd mentioned this pull request Jul 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Zoxc @michaelwoerister @TimNN @bors @Mark-Simulacrum @pietroalbini